This PR cannot be merged: expecting exactly one kind/ label

This PR cannot be merged: expecting exactly one kind/ label

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

/kind bug

Processing o(n) resources every time could result in any unwanted bitter side effect for the controller deployment

O(n) isn't a change from before, but the way this is written now ensures that only one scan i happening at a time. Suppose "n" is huge and it takes 10 minutes to scan the lister cache.

This approach would just take 10:30 to expose the metric. The previous approach would leak go routines every time we update a single resources status. So in addition to being O(n) in runtime, it also might leak O(n) go routines[1] if all n pipelines are actively running.

[1] - I'm assuming there are roughly a constant number of status updates, but this could be much higher given that there is no deduplication by key.

If resync could be a problem, isn't it a general reconciliation problem?

It is. For global resyncs we jitter to avoid starving real work for a period of time. The default resync period is also every 10 hours to mitigate this, but that seems overly coarse for metrics resolution to be useful.

/retest

Thank you @mattmoor for the explanation.

O(n) isn't a change from before, but the way this is written now ensures that only one scan i happening at a time. Suppose "n" is huge and it takes 10 minutes to scan the lister cache.

O(n) holds true, sorry to rephrase it, I meant every time vs only when something changes. :)

The previous approach would leak go routines every time we update a single resources status.

True, as there wasn't serialization in place (frankly I was thinking to serialize it during the implementation using queue but later on felt that would be over-engineering) thus it could trigger the create stale metrics. However, the current PR approach deals better in that case.

[2]

The previous method was prone to stale
data because it was driven by the informer cache immediately following
writes through the client, and might not fix itself since the status
may not change (even on resync every 10h). With this change it should
exhibit the correct value within 30s + {informer delay}, where the 30s
is configurable on the Recorders.

On a different note, I'm still not able to get comment[2] and bit curious, would you like to elaborate on it?
As Comment mentions

Now, informers are fast, but in all likelihood the lister that is being used to drive this metric hasn't received the updated version of the resource from the API Server, so unless the informer cache's copy is being modified (a no no) then the cache won't reflect what it's clearly (given the if updated check) trying to capture.

However, I'm under the impression that either through the list or watch the updated copy of the resource is placed into the cache first then informer is notified about the updated resource which triggers reconciliation. Thus lister in the reconciler() refers the same copy from the cache which is up to the update. Is it right?

I'm under the impression that either through the list or watch the updated copy of the resource is placed into the cache first then informer is notified about the updated resource which triggers reconciliation

Yes, the informer model uses watches, lists, and changes on those lists to simulate events by observing changes on those lists through the watch. The premise of using the informer cache for reads during reconciliation is that the data is never a stale read of the change that motivated the reconciliation (because the event was generated by an update to the informer cache). It could still be stale if subsequent reads came in, but this is why "in-flight" work isn't deduplicated with "queued" work (e.g. even if a key is being processed it can be duplicated in the work queue).

However, in the case at HEAD, the metrics logic is keying off of if updated, which is NOT an informer event (e.g. observer-side where lister caches have been updated). So the sequence of events where this works is:

1. update status (updated = true)
2. informer QUICKLY observes the change and updates the lister cache (the key is also requeued)
3. metrics go routine is spawned and sees correct data
4. The key is processed again, nothing to do (updated = false)
5. metrics go routine is not spawned because nothing changed.

2. -> 3. is the informer event happening first by chance, but there is no causal link here. We aren't updating metrics because the informer cache reflects the updated state.

The sequence of events where this doesn't work is:

1. update status (updated = true)
2. metrics go routine is spawned and sees INCORRECT data
3. informer SLOWLY observes the change and updates the lister cache (the key is also requeued)
4. The key is processed again, nothing to do (updated = false)
5. metrics go routine is not spawned because nothing changed.

In the former case we get really lucky (informers are fast, but not "faster than a local go routine" fast). There used to be a bug that meant we didn't have to get lucky, but that is fixed at HEAD. Two other notable aspects to this:

1. The metrics data is never reevaluated on a nop resync, so bad data stays bad until there's a change (and we get lucky enough that there isn't new bad data).
2. When the controller process starts up, it won't surface this metric data until it observes a change.

Fundamentally what's at HEAD is "edge triggered" (keying off of deltas), vs. "level based" (changes that trigger more holistic reconsideration). It's not that "level based" is the only correct way to do things and it can seem wasteful to reconsider everything when we think we know exactly what changed, but it can be much more forgiving of errors and dropped events. I had some slides on this that it seems I never shared publicly and are locked up at Google somewhere 😞

@vdemeester:

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, hrishin, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@mattmoor I'm afraid this will need a rebase :)

Rebased.

(will need a fresh /lgtm)

I'll work on rebasing the others now as well.

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

/lgtm